-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VIF and GVIF #300
VIF and GVIF #300
Conversation
palday
commented
Sep 6, 2023
•
edited
Loading
edited
- still needs more extensive tests, but I haven't had a chance to see what it would take to fake something reasonable
Also there seem to remain a few uncovered lines (apart from the |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd generally be a lot more comfortable if there was some kind of reproducible way to get these reference values (even if it's in a script that doesn't actually get automatically executed during tests). but otherwise looks good
test/vif.jl
Outdated
@@ -44,6 +44,10 @@ StatsModels.formula(model::Duncan) = model.formula | |||
@test vif(MyRegressionModel3()) ≈ [1, 1] | |||
end | |||
|
|||
# Reference values from | |||
# https://github.com/palday/MixedModelsExtras.jl/blob/6db061cb856301c04b978101d4c53b90c890715b/test/vif.jl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you copy the relevant parts from that file instead? It's weird to rely on MixedModelsExtras for that, especially if this PR is supposed to provide the same features more upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, the relevant parts of that file are just the reference values. I'm planning on writing out the R code to do it again, which is why I hadn't merged this PR yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in 96686ab